Conversation
Nothing tested really, but the extension gets loaded and we did find build issues!
There was a problem hiding this comment.
Pull Request Overview
This PR implements web extension support for the VS Code Git commit message extension, allowing it to run in both desktop and web environments. The changes introduce dual webpack configurations, refactor Node.js-specific functionality to be conditionally available, and add comprehensive testing infrastructure.
- Refactored build system to support both Node.js and web worker targets
- Added conditional execution of Git commands based on runtime environment
- Implemented cross-platform path utilities and comprehensive testing
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| webpack.config.js | Creates dual build configurations for desktop and web extension targets |
| src/utils.ts | Adds cross-platform path utilities and conditional Node.js module loading |
| src/verbosecommits.ts | Implements conditional Git command execution for web compatibility |
| src/getgitbranch.ts | Updates Git branch detection to work in both desktop and web contexts |
| src/quickfix.ts | Replaces Node.js path module with cross-platform utility |
| src/messages.ts | Replaces Node.js path module with cross-platform utility |
| src/diagnostics.ts | Replaces Node.js path module with cross-platform utility |
| src/extension.ts | Adds API export for testing purposes |
| src/test/suite/extensionIntegration.test.ts | Adds comprehensive Git branch detection tests |
| src/test/suite-web/index.ts | Implements web extension test harness |
| package.json | Configures dual extension support and web testing infrastructure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
webpack.config.js
Outdated
| { | ||
| require.resolve('assert') | ||
| } | ||
|
|
There was a problem hiding this comment.
This code block appears to be orphaned and serves no purpose. It should be removed as it's not being used anywhere in the configuration.
| { | |
| require.resolve('assert') | |
| } |
src/utils.ts
Outdated
| * @returns The file name portion of the path. | ||
| */ | ||
| export function basename(filePath: string): string { | ||
| const parts = filePath.split(/[/]+/); |
There was a problem hiding this comment.
The regex /[/]+/ only handles forward slashes but the comment mentions handling both / and \ separators. It should be /[/\\]+/ to properly handle both Unix and Windows path separators.
src/utils.ts
Outdated
| * @returns The directory portion of the path. | ||
| */ | ||
| export function dirname(filePath: string): string { | ||
| const parts = filePath.split(/[/]+/); |
There was a problem hiding this comment.
The regex /[/]+/ only handles forward slashes but the comment mentions handling both / and \ separators. It should be /[/\\]+/ to properly handle both Unix and Windows path separators.
| const { stdout } = await utils.execFile!( | ||
| "git", | ||
| ["rev-parse", "--abbrev-ref", "HEAD"], | ||
| ["branch", "--show-current"], |
There was a problem hiding this comment.
The change from rev-parse --abbrev-ref HEAD to branch --show-current may cause compatibility issues. The --show-current flag was introduced in Git 2.22.0 (2019), while rev-parse --abbrev-ref HEAD works with much older Git versions. Consider keeping the original command for better backward compatibility.
| ["branch", "--show-current"], | |
| ["rev-parse", "--abbrev-ref", "HEAD"], |
There was a problem hiding this comment.
The existing command doesn't work on new repos where there is no HEAD.
1 participant
No description provided.